ci: publish a GitHub Release on every release#52
Conversation
|
Claude finished @uipreliga's task in 1m 20s —— View job Code Review in Progress
|
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:52
Scope: pr:52 · branch ci/publish-github-releases · 3454cbf · 2026-07-24T19:17Z · workflow variant
Change class: complex — adds a new CI publish step with embedded bash + Python control flow (CHANGELOG regex slicing, notes fallback branch, app-token handling, continue-on-error semantics) on the release path; correctness requires reasoning, and the rule says choose complex when uncertain
coder_eval is in excellent shape — architecture, error handling, test health, and the evaluation harness score a clean 10/10 with zero critical/high/medium findings, and nothing in this review can change a task's score or final_status for identical agent output; the only real risks are confined to the new "Publish GitHub Release" step in .github/workflows/release.yml, where unreviewed commit-message-derived CHANGELOG text becomes a first-party Release body (content/link spoofing), a ruleset-bypass app token is exported where a job-scoped GITHUB_TOKEN would suffice, and a locale-dependent read_text() could silently no-op the release under continue-on-error — bottom line: ship it, then tighten the release-publishing surface.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 9.9 / 10 | 0 | 0 | 0 | 1 | Inaccurate/redundant block comments on the new Publish GitHub Release step |
| 2. Type Safety | 9.9 / 10 | 0 | 0 | 0 | 1 | Notes-extraction heredoc reads/writes CHANGELOG.md without explicit encoding="utf-8" |
| 3. Test Health | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 4. Security | 9.8 / 10 | 0 | 0 | 0 | 2 | Ruleset-bypass GitHub App token exported as GH_TOKEN where a job-scoped GITHUB_TOKEN would suffice (least privilege) |
| 5. Architecture & Design | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 6. Error Handling & Resilience | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 7. API Surface & Maintainability | 9.9 / 10 | 0 | 0 | 0 | 1 | Deferred harness note overstates the sdist leak: node_modules only reproduces in a dirty local tree, not the published sdist |
| 8. Evaluation Harness Quality | 10 / 10 | 0 | 0 | 0 | 0 | — |
Overall Score: 9.9 / 10 · Weakest Axis: Security at 9.8 / 10
Totals: 🔴 0 · 🟠 0 · 🟡 0 · 🔵 5 across 8 axes.
Blockers
None.
Non-blocking, but please consider before merge
None.
Nits
- [Axis 1] Inaccurate/redundant block comments on the new Publish GitHub Release step (
.github/workflows/release.yml:259) — Line 259 reads# the sdist that "Build wheel + sdist" publishes to PyPI two steps below.TheBuild wheel + sdiststep is at line 282 — the immediately next step, not two below — and it does not publish to PyPI at all: it runsuv build, the artifact is uploaded at line 289, and the actual publish happens in the separatepublish-pypijob (line 365). Reword to something like# the sdist that the next step ("Build wheel + sdist") produces and the publish-pypi job uploads.The underlying claim about hatchling's default file selection is correct —pyproject.tomlhas[tool.hatch.build.targets.wheel](line 122) but no[tool.hatch.build.targets.sdist]section — only the positional wording is wrong. - [Axis 2] Notes-extraction heredoc reads/writes CHANGELOG.md without explicit encoding="utf-8" (
.github/workflows/release.yml:266) — Line 266 istext = pathlib.Path("CHANGELOG.md").read_text()and line 268 ispathlib.Path(os.environ["NOTES_FILE"]).write_text(m.group(1).strip() if m else "")— both rely on the ambient locale encoding. CHANGELOG.md verifiably contains non-ASCII (→,—,✓), so under a non-UTF-8 locale theread_text()raisesUnicodeDecodeError,set -euo pipefailaborts the step, andcontinue-on-error: true(line 246) swallows it — no Release is published and the job still goes green. PEP 538 coercion makes this latent rather than live on ubuntu-latest, but the fix is one keyword:read_text(encoding="utf-8")/write_text(..., encoding="utf-8"). Same nit applies to the pre-existing sibling heredoc at lines 163/166, so fixing both keeps the file consistent. - [Axis 4] Ruleset-bypass GitHub App token exported as GH_TOKEN where a job-scoped GITHUB_TOKEN would suffice (least privilege) (
.github/workflows/release.yml:250) — Line 250 exports the GitHub App installation token into the new step:GH_TOKEN: ${{ steps.app-token.outputs.token }}, justified by the comment on lines 248-249 ("The app token, not GITHUB_TOKEN: the workflow's permissions are contents: read, and the release app already carries contents: write."). That app token is the one the workflow header (lines 40-43) describes as having the ruleset bypass onmain— strictly more authority thangh release createneeds. A narrower alternative exists and is not used: add a job-levelpermissions:block grantingcontents: write(alongside the existingpackages: write) and setGH_TOKEN: ${{ github.token }}for this step; the ephemeral, repo-scoped GITHUB_TOKEN can create releases and cannot bypass the branch ruleset. Mitigating facts that hold this at Low rather than higher:actions/create-github-app-tokencallscore.setSecretso the value is log-masked;actions/checkout(line 87) already persists the same token in.git/configfor the whole job, so every step in this job can already read it; and the step'srun:body (lines 254-280) executes no third-party code — onlypython3from a quoted heredoc and the preinstalledgh, neither of which echoes the environment. So this is hygiene, not an exploitable hole. Verified-clean adjacent claims (no finding): (a) the comment on lines 251-252 ("Passed via env (not interpolated into the script) per GitHub's injection guidance") is TRUE — the new step'srun:body contains zero${{ }}expressions; the only two${{ }}-into-run:interpolations in the whole file are pre-existing at line 215 (git push origin main "v${{ steps.release.outputs.version }}") and line 307 (echo '${{ github.repository_owner }}'), neither attacker-controlled; (b)steps.release.outputs.versionoriginates from$PSR version --print(line 146), i.e. semantic-release's own PEP440 computation overpyproject.toml, and is not attacker-injectable; (c) the heredoc at line 263 IS single-quoted (<<'PY'), so no shell expansion into the Python body, andre.escape(version)on line 267 additionally blocks regex injection; (d) the sdist-leak mitigation on lines 256-260 is correct — I empirically built a scratch hatchling sdist and confirmed an untracked, non-root-gitignored file at the project root IS packaged, so writing to${RUNNER_TEMP}/release-notes.mdrather than the repo root genuinely prevents the notes file from reaching the PyPI sdist. CVSS:3.1/AV:L/AC:H/PR:H/UI:N/S:U/C:N/I:L/A:N - [Axis 4] Unreviewed commit-message-derived CHANGELOG text published verbatim as the GitHub Release body (content/link spoofing on a first-party distribution surface) (
.github/workflows/release.yml:276) — Lines 263-280 slice a CHANGELOG.md section and publish it unmodified as a GitHub Release body:pathlib.Path(os.environ["NOTES_FILE"]).write_text(m.group(1).strip() if m else "")(line 268), thenNOTES=(--notes-file "$NOTES_FILE")(line 271) feedinggh release create "v${VERSION}" --title "v${VERSION}" --verify-tag --latest "${NOTES[@]}"(lines 276-280). Trace the content: CHANGELOG.md is regenerated by semantic-release during this same run ($PSR version "--$BUMP" ... --changelog, line 144) from commit subjects since the last tag, and the amended commit is pushed straight tomainwith the app token's ruleset bypass — so that text reaches a published Release with no PR review of the rendered result. The current CHANGELOG shows commit subjects going in verbatim as markdown, e.g.- Code review fixes for welch-t-test-exact ([#38](https://github.com/UiPath/coder_eval/pull/38), ...). A contributor whose PR title/commit subject is approved for its code (not its markdown) can therefore land a link like[install from here](https://evil.example)into a first-party-looking Release body, which GitHub also fans out in release-notification emails and is what Marketplace listings render (the stated motivation on lines 232-233). Same class applies to the--generate-notesfallback (line 274), which GitHub builds from PR titles. Impact is capped at content/link spoofing — GitHub sanitizes raw HTML/script in release bodies, so no XSS — and this PR is what first creates the surface (no Release existed before). Recommendation: this is acceptable-with-eyes-open for a repo with mandatory review, but state it: either add a human gate (e.g. create the release as a draft with--draftand publish after a glance) or strip/neutralize bare markdown links in the sliced notes before writing$NOTES_FILE. If neither, record the accepted risk in the step comment next to lines 235-237 so a future reader does not assume the notes are trusted content. CVSS:3.1/AV:N/AC:H/PR:L/UI:R/S:C/C:N/I:L/A:N - [Axis 7] Deferred harness note overstates the sdist leak: node_modules only reproduces in a dirty local tree, not the published sdist (
.claude/harness-candidates.md:37) — Lines 36-39 read:
`coder_eval-X.Y.Z/release-notes.md` (verified by building it), and the sdist
*already* ships `evalboard/node_modules/**` even though that path is git-ignored
via `evalboard/.gitignore` — so `.gitignore` is not a reliable exclusion
mechanism here. Nothing guards what reaches PyPI today.
The mechanism is real — I confirmed it with hatchling's own selector (SdistBuilder(".").recurse_included_files() yields evalboard/node_modules/.modules.yaml, evalboard/node_modules/.bin/autoprefixer, evalboard/.next/server/vendor-chunks/*, because hatchling honors only the root .gitignore, not the nested evalboard/.gitignore). But it does not describe the published artifact: release.yml's job never runs npm/pnpm install, so a fresh CI checkout has no evalboard/node_modules at uv build time, and the actual PyPI sdist for 0.8.9 is 7,540,654 bytes (https://pypi.org/pypi/coder-eval/0.8.9/json) versus 526 MB for evalboard/node_modules locally. Framing a dirty-worktree artifact as "what reaches PyPI today" will make whoever picks this item up mis-prioritize it. Reword to scope the claim, e.g. "a local uv build sweeps in evalboard/node_modules/** and evalboard/.next/** because hatchling reads only the root .gitignore; CI is currently spared only because it never installs node deps — so any untracked file a workflow leaves in the tree before uv build does reach PyPI (which is why the new step writes notes to $RUNNER_TEMP)."
What's Missing
Tests:
- 🟡 Tests — 🟠
action.yml: this PR hand-fixes a pin drift (0.8.6→0.8.9) that nothing detects. The invariant is mechanical and always true at rest —action.yml'sversion:default must equalpyproject.toml's[project].version(release.yml:200-201 sed-bumps it in the release commit) — yet no test, lint rule (CE0nn), or CI check asserts it, which is exactly whyaction.ymlshipped in 74db6fa with a stale0.8.6while main was already0.8.9. Add a one-line parity test (or aCE0nndoc-surface rule alongside CE027–CE031) so@v0consumers can never silently install a version other than the tag they pinned; also record it in.claude/harness-candidates.md, which this PR touches but only records the sdist item. (trigger: action.yml) - 🟡 Tests — 🟠
.github/workflows/release.yml:238-280: the new step is unreachable by the workflow's own dry-run affordance. The header (lines 17-22) advertises a branch dispatch as the way to rehearse a release, but the step is gatedif: steps.mode.outputs.prerelease != 'true' && steps.release.outputs.version != '', so a prerelease dispatch skips it entirely — the ~27 lines of new bash + inline Python (CHANGELOG slice,$NOTES_FILEempty-check,gh release create --verify-tag) will execute for the first time against productionmain, after the tag has already been pushed. At minimum, unit-test the notes-extraction heredoc's regex against the realCHANGELOG.md(it is pure text processing and needs no network), or let the prerelease path run it with--draft/gh release create --dry-run-equivalent so the logic is exercised before it matters. (trigger: .github/workflows/release.yml) - 🔵 Tests — 🔵
.github/workflows/release.yml:254-280: the PR adds ~27 lines of shell + inline Python to a workflow, and the repo has no static gate over.github/workflows/**—grep -rn 'actionlint|shellcheck|yamllint'across.github/,Makefile, and.pre-commit-config.yamlreturns nothing, andmake verifynever looks at workflow YAML.actionlint(which runs shellcheck overrun:bodies and pyflakes over inlinepython3heredocs) would mechanically cover the encoding, quoting, and array-expansion shapes reviewed by hand here, and would subsume the deferredCE026SHA-pinning candidate already sitting in.claude/harness-candidates.md:14-17. (trigger: .github/workflows/release.yml)
Daily/nightly:
- 🟡 Daily/nightly pipeline impact — 🟠
.github/workflows/release.yml:238: the new step is inserted into the critical release path, beforeBuild wheel + sdist(282) andUpload dist for PyPI publish(289), inside a 15-minutetimeout-minutesjob budget — and the PR states no blast radius for the release/nightly contract.continue-on-error: truecovers a non-zero exit but NOT a hung or slowghAPI call: that consumes the job budget and cancels the job, producing precisely the failure mode the step's own comment (lines 240-245) says must never happen —mainbumped,vX.Y.Z+ movingv0tags pushed,action.ymlpinned to a version that never reached PyPI, and the ADO nightly'scoderEvalVersionpin unresolvable. Moving the step to afterUpload dist for PyPI publishremoves the risk entirely at zero cost (the tag it verifies is already on the remote). (trigger: .github/workflows/release.yml)
Downstream consumers:
- 🔵 Downstream consumers — 🟡
.github/workflows/release.yml:267: the notes slice hard-codes semantic-release's current changelog rendering (^## v{version}\b … (?=^## v|\Z)), but nothing couples the two.pyproject.toml's[tool.semantic_release.changelog](line 369) usesdefault_templates; a template/modechange, av-less heading, or a futurechangelog.exclude_commit_patternstweak silently degrades every release body to--generate-notes— and the only signal is a::warning::(line 273) inside acontinue-on-error: truestep, so the job still goes green and nobody notices the Release notes stopped matching the CHANGELOG. Either assert the regex againstCHANGELOG.mdin a test, or make the miss loud (fail the step rather than warn) now that the tag is already pushed and a retry is cheap. (trigger: .github/workflows/release.yml) - 🔵 Downstream consumers — 🟡
.github/workflows/release.yml:238-280: nothing backfills the surface this PR creates.git ls-remote --tags originshowsv0.8.1…v0.8.9with nov0tag, andgh release listreturns zero releases — so the Releases page will begin at whatever version is cut next, and theuses: UiPath/coder_eval@v0form documented inREADME.md:105,142anddocs/CI_GATE.md:22,67stays unresolvable until then. Since the stated motivation is Marketplace listings (line 233), also note the remaining manual step:gh release createcannot tick GitHub's "Publish this Action to the Marketplace" box, so the listing still requires a one-time human action that no comment or doc records. (trigger: .github/workflows/release.yml)
Parallel paths:
- 🔵 Parallel paths — 🟡
.github/workflows/release.yml:3-22: the file's own header block is the SSOT for what a release does, and it was not updated. Line 3-4 still enumerates the outputs as "bump the version, tag, build, and publish to public PyPI (wheel) and GHCR (agent image)" with no mention of a GitHub Release, and the PRERELEASE paragraph's explicit exclusion list (lines 19-20, "It does NOT move:latest, tag, commit, or push to main") was not extended with "or create a GitHub Release" — even though that asymmetry is a deliberate design decision of this PR. A reader of the header now gets a materially incomplete picture of the release contract. (trigger: .github/workflows/release.yml) - 🔵 Parallel paths — 🔵
.github/workflows/release.yml:163,166: the pre-existing prerelease-stamping heredoc has the identicalread_text()/write_text()-without-encoding=shape as the new notes heredoc, and was not updated alongside it; the repo already enforces this pattern for Python source via theread_text_explicit_encodinglint rule, but that rule never sees code embedded in workflow YAML. Fix both in one pass so the file is internally consistent (and so an eventual actionlint/CE0nnextension has nothing left to flag). (trigger: .github/workflows/release.yml) (restates: Axis 2: Notes-extraction heredoc reads/writes CHANGELOG.md without explicit encoding="utf-8")
Harness & Lint Improvements
Static checks (lint / type):
- [ce-lint] CE032 — run the existing AST rules over Python embedded in
.github/workflows/*.yml. Root cause of the encoding finding is surface, not pattern: CE008/CE009/CE010 already forbid unencodedread_text/open/subprocess.run, buttests/lint/runner.py::check_pathsonly walks*.pyundersrc/, so Python living inside arun:heredoc is invisible to ruff, pyright, pytest coverage and the CE runner. Addtests/lint/workflow_python.py: scan.github/workflows/*.ymlfor interpreter heredocs (python3 - <<'PY' … PY),textwrap.dedentthe body,ast.parseit, and feed the tree through the existingALL_RULES(at minimum the encoding trio), mapping violation line numbers back to the YAML line (heredoc start + offset). Like CE027–CE031 it reasons over non-.pyfiles, so wire it astests/test_custom_lint.py::TestCE032WorkflowEmbeddedPythonrather than as aBaseRulein the runner. CE032 is the next free number. I prototyped the extractor (~30 lines) against the live tree: it parses all 6 heredocs successfully and reports exactlyrelease.yml:266(read_text),release.yml:268(write_text), plus the pre-existingrelease.yml:163/166andpublish-testpypi.yml:79/82. Prevents: Finding #2 (release.yml:266/268—read_text()/write_text()withoutencoding="utf-8"on a CHANGELOG.md that verifiably contains→ — ✓), and the four identical pre-existing instances the finding calls out as siblings. - [ce-lint] Extend CE008 from
read_texttowrite_text.tests/lint/rules/read_text_explicit_encoding.pymatches only.read_text(...); nothing in the repo forbidsPath.write_text(...)withoutencoding=.src/happens to be clean today (all 19 call sites passencoding=, verified by AST walk — several as trailing kwargs on multi-line calls), so this is purely a rot guard on an invariant currently held by convention. One-line change: matchnode.func.attr in {"read_text", "write_text"}and reword the message; the docstring already says "Same root cause as CE008 (read_text/write_text)" inopen_explicit_encoding.py, i.e. the intent was always both. Prevents: The write half of finding #2 (release.yml:268) once CE032 routes workflow heredocs through the same rules, and future unencodedwrite_textinsrc/. - [ruff] Consider
PLW1514(unspecified-encoding) instead of hand-maintaining CE008/CE009/CE010. Ruff's native rule coversopen,read_text,write_text,subprocesstext mode in one flag and has an autofix. Caveat that keeps this second-choice, not first:ruff rule PLW1514reports it is preview-only (uv run ruff check --select PLW1514warns "has no effect because preview is not enabled"), so adopting it meanslint.preview = true, which opts every other selected rule into preview churn. Recommendation: prefer the CE008 extension above for stability, and revisit PLW1514 when it stabilizes. Note ruff cannot see YAML either way, so it never replaces CE032. Prevents: Same class as finding #2, plustests/(which the CE runner does not scan) — recorded here so the choice of custom rule over the ruff-native option is deliberate rather than an oversight. - [ce-lint] CE033 — interpreter heredocs in
.github/workflows/**must use a quoted delimiter (<<'PY', not<<PY). With an unquoted tag, the shell expands$VAR/${{ }}into the program text before the interpreter sees it, so a value containing a quote or newline breaks out of the Python string literal — the exact hazard the reviewed step's own comment (release.yml:251-252, "Passed via env … per GitHub's injection guidance") sets out to avoid, and which the review verified clean for the new step. There is a live violation in-tree:publish-testpypi.yml:67usespython3 - <<PYand interpolatesf'{key} = "${DEV_VERSION}"'(line 79) directly into the Python source. Regex-detectable in ~10 lines; naturally ships alongside CE032, which needs the same heredoc scanner (and whoseast.parseis only sound on quoted bodies). Prevents: Hardens the property finding #4 verified by hand atrelease.yml:263(heredoc is<<'PY', so no shell expansion into the Python body) so it cannot regress, and fixes the existing counter-example atpublish-testpypi.yml:67. - [bandit-codeql] Add
zizmor(GitHub Actions static analyzer) +actionlinttomake verifyand thepr-checksworkflow.grepconfirms neither is present today, so no static analysis whatsoever runs over.github/workflows/**— every workflow finding in this review was caught by a human reading YAML. Three zizmor rules map one-to-one onto the findings:excessive-permissions(flags the missing job-levelpermissions: contents: writethat forces the ruleset-bypass app token intoenv:atrelease.yml:250),artipacked(flagsactions/checkoutpersisting that same credential in.git/configfor the whole job — cited in finding #4 as the mitigating-but-unhygienic fact atrelease.yml:87), andtemplate-injection(the${{ }}-into-run:interpolations atrelease.yml:215and:307, verified benign here but unguarded against future edits). Run as a non-blocking annotation job first, then promote to blocking once the existing backlog is triaged. Prevents: Finding #4 (release.yml:250least-privilege token) directly, and pre-emptively the injection class finding #4 had to verify by hand across the whole file.
Harness improvements (not statically reachable):
- Lift the release-notes slicing out of the heredoc into a tested module. Replace
release.yml:263-269withpython3 scripts/release_notes.py(orpython -m tests.tools.release_notes) exposingextract_section(changelog_text, version) -> str, plus unit tests over fixture CHANGELOGs: version present, version absent (empty →--generate-notesfallback), section is the last one in the file (\Zbranch), a version string containing regex metacharacters, and a fixture carrying the real→ — ✓bytes. The regex at line 267 (^## v{re.escape(version)}\b.*?$(.*?)(?=^## v|\Z)) is non-trivial, has three failure modes, and is currently exercised only in production during an actual release. Why not static: The defects are behavioral — wrong slice boundary, empty notes, decode failure on real bytes — and need execution against fixture inputs. Code inside a YAML heredoc is also structurally unreachable to ruff, pyright, pytest and coverage; CE032 above only recovers the lint layer, never the test layer. Moving the code is the only fix that restores all four. Prevents: Finding #2 (encoding, which a non-ASCII fixture test fails on immediately) and finding #1 — a module with a docstring removes the drifting positional prose ("two steps below", "publishes to PyPI") that the inline comment atrelease.yml:259got wrong; that wording error is genuinely not mechanically detectable, so relocating the surface is the only preventive move available. - Make
continue-on-error: truesteps loud. Add anid:to the Publish-GitHub-Release step and a following always()-guarded step that fails-soft with::error(or opens an issue / posts to the release channel) whensteps.<id>.outcome != 'success'; complement with a scheduled check asserting every published tagvX.Y.Zhas a corresponding GitHub Release. Todayrelease.yml:246swallows any failure of the new step and the job still goes green. Why not static: Requires the runtime outcome of a real workflow run (and the live tag/release state on GitHub); no analyzer can know whether the step failed. Prevents: The silent-failure amplifier on finding #2 — under a non-UTF-8 locale theUnicodeDecodeErroraborts the step, no Release is published, and nothing anywhere reports it. - Add an sdist manifest guard to CI. A job that runs
uv build --sdistand asserts the tarball's member list against an allowlist: noevalboard/node_modules/**, noevalboard/.next/**, no unexpected files at the package root. Optionally back it with an explicit[tool.hatch.build.targets.sdist]include/exclude inpyproject.toml(which today has only thewheeltarget at line 122), so the contract is declared rather than inferred from hatchling's default file selection. Why not static: Hatchling's selector is the ground truth and only reveals itself by running — the review had to build a scratch sdist to establish that untracked root files are packaged while nested.gitignores are not honored. No grep overpyproject.tomlor.gitignorepredicts that. Prevents: Finding #5 (.claude/harness-candidates.md:37) — a machine-produced manifest replaces the disputed prose about whethernode_modulesreaches PyPI (local dirty build vs. published 0.8.9 artifact), and future readers get a fact instead of a conflation. Also permanently enforces the invariant the new step's$RUNNER_TEMPcomment (release.yml:256-260) currently protects by convention only. - Gate the Release body's provenance. Either (a) create the Release with
--draftand publish after a glance, or (b) add apr-checksjob rejecting markdown link syntax / bare URLs in PR titles — the squash subject becomes the CHANGELOG line (release.yml:144) and thence the verbatim Release body (release.yml:271-280). If neither is adopted, record the accepted risk in the step comment next torelease.yml:235-237so the notes are not mistaken for reviewed content. Why not static: The untrusted text lives in PR/commit metadata, not in any file a code scanner reads; option (b) needs the GitHub PR context at CI time, option (a) is a human gate by definition. Prevents: Finding #4b (release.yml:276) — an approved-for-code PR title carrying[install from here](https://evil.example)landing in a first-party Release body that GitHub fans out over email and Marketplace listings.
Top 5 Priority Actions
- Gate or sanitize the Release body at .github/workflows/release.yml:276 — the CHANGELOG slice written at line 268 is derived from commit subjects and published verbatim to a first-party surface (and release-notification emails) with no review of the rendered markdown, so either create it with --draft, strip bare markdown links before writing $NOTES_FILE, or record the accepted risk in the step comment near lines 235-237.
- Add encoding="utf-8" to read_text()/write_text() at .github/workflows/release.yml:266,268 (and the pre-existing sibling heredoc at lines 163/166) — CHANGELOG.md contains non-ASCII, so a non-UTF-8 locale raises UnicodeDecodeError, set -euo pipefail aborts the step, and continue-on-error: true (line 246) swallows it: no Release published, job still green.
- Drop to least privilege at .github/workflows/release.yml:250 — replace the ruleset-bypass GitHub App token with a job-level
permissions: contents: writeplus GH_TOKEN: ${{ github.token }}, since gh release create needs none of the branch-protection bypass authority the app token carries. - Rescope the deferred sdist note at .claude/harness-candidates.md:37 — it presents a dirty-local-worktree artifact (evalboard/node_modules swept in because hatchling honors only the root .gitignore) as "what reaches PyPI today", which is false for CI (the 0.8.9 sdist is 7.5 MB) and will make whoever picks the item up mis-prioritize it.
- Fix the misleading step comment at .github/workflows/release.yml:259 — "Build wheel + sdist" is the immediately next step (line 282), not two below, and it does not publish to PyPI (uv build only; the publish-pypi job at line 365 does), so reword while keeping the correct hatchling default-selection rationale.
Stats: 0 🔴 · 0 🟠 · 0 🟡 · 5 🔵 across 8 axes reviewed.
bai-uipath
left a comment
There was a problem hiding this comment.
Fix what you agree with, otherwise lgtm
-
Move the step after
Upload dist for PyPI publish(release.yml:238→ after:294). It currently sits at the earliest legal point after the tag push, which maximizes the window where a Release exists for a version that isn't installable. -
continue-on-errorhere is silent (release.yml:246). The rationale is right, but agh release createfailure leaves the job green with only a step-level annotation that nobody reads on a release run — so the failure class this PR fixes (no Releases) can recur unnoticed.
Reviewer items acted on: - Move "Publish GitHub Release" after "Build wheel + sdist" and "Upload dist for PyPI publish". Those are the last steps that can still fail for an already-tagged version, so a Release no longer announces a version whose artifacts never built, and a hung `gh` call can no longer eat the 15-minute job budget before the artifacts are safe. - Make the swallowed `continue-on-error` failure loud: a new "Flag missing GitHub Release" step re-raises it as an ::error annotation plus a run-summary block with the by-hand recovery command, without failing the job (which would skip the `needs: release` PyPI publish). - Extract the CHANGELOG notes slicing out of the `run:` heredoc into .github/scripts/release_notes.py, covered by tests/test_release_notes.py. Heredoc code is invisible to ruff, pyright, pytest and coverage, and this regex has three failure modes on a path that runs once per release against production main with no rehearsal (the prerelease dispatch skips the step). Tests pin the load-bearing \b (0.8.1 must not slice the v0.8.10 section), the \Z branch, re.escape, the empty-file fallback contract, and couple the regex to semantic-release's real rendering of CHANGELOG.md. - Pin encoding="utf-8" on every read_text/write_text in workflow-embedded Python (release.yml, publish-testpypi.yml). CHANGELOG.md carries non-ASCII, so a non-UTF-8 locale raised UnicodeDecodeError inside a continue-on-error step: no Release, still green. - Quote the publish-testpypi.yml heredoc delimiter (<<'PY') and read DEV_VERSION from os.environ instead of expanding it into the Python source. - Add tests/test_action_version_pin.py: action.yml's `version:` default must equal pyproject.toml's version, and the `# <-- kept in sync` sed anchor must be present and unique. Nothing detected the 0.8.6-vs-0.8.9 drift this PR hand-fixed, which would have had @v0 consumers installing a version other than the tag they pinned. - Fix the misleading step comment ("two steps below", "publishes to PyPI") and update the workflow header, which still omitted the GitHub Release from both the outputs list and the PRERELEASE exclusion list. - Record the accepted risk on the Release body: notes render squashed PR titles, so a first-party surface carries text reviewed as code, not markdown. Also note that Marketplace listing needs a one-time manual checkbox. - Widen the Makefile lint scope to .github/scripts/ so extracted release tooling is actually linted. Rescoped the deferred sdist note in .claude/harness-candidates.md: it claimed the sdist "already ships evalboard/node_modules/**" to PyPI. Verified false for published artifacts — the 0.8.9 and 0.8.2 sdists on PyPI are ~7.5 MB with zero node_modules entries, because CI never runs pnpm install. It reproduces only in a developed local worktree (135 MB, 8520 files), since hatchling honors just the root .gitignore. The live hazard is untracked files a workflow leaves at the root, which is what the $RUNNER_TEMP note guards. Also recorded CE032/CE033 and actionlint/zizmor as deferred candidates. Declined: swapping the app token for GITHUB_TOKEN + job-level contents: write. actions/checkout already persists the same app token in .git/config for every step in the job, so scoping it out of this one step's env buys no isolation while adding a second write credential. Rationale recorded in the step comment. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Review responses — addressed in f2329ddThanks both. Everything I agreed with is in Fixed
The extraction found a real bug. The trailing Declined, with reasoningNit 3 — swap the app token for Nit 4 — One correction: the sdist /
|
| sdist | size | files | node_modules entries |
|---|---|---|---|
PyPI coder_eval-0.8.9.tar.gz |
7,540,654 B | 555 | 0 |
PyPI coder_eval-0.8.2.tar.gz |
7,435,290 B | 536 | 0 |
local uv build --sdist, developed worktree |
135,399,741 B | 9,280 | 8,520 |
CI is spared because release.yml never runs npm/pnpm install, so those paths don't exist on the runner at uv build time. The mechanism is real (hatchling honors only the root .gitignore, so the nested evalboard/.gitignore is ignored — evalboard/.next/** leaks too, 190 files), but it reproduces only locally. Worth adding: a dirty-tree release wouldn't silently ship third-party JS either, since 135 MB exceeds PyPI's 100 MB per-file limit — the failure mode is a broken release, not a stealth one.
The genuinely live hazard is the other half: untracked files a workflow leaves at the repo root, which hatchling does package. That's what the $RUNNER_TEMP guard is for, and it's enforced by convention only. .claude/harness-candidates.md now separates the two.
Deferred to .claude/harness-candidates.md
Per the repo's convention for deferred guardrails, rather than expanding this PR: CE032 (run the existing CE008/009/010 AST rules over Python embedded in workflow YAML — plus extending CE008 to write_text), CE033 (quoted heredoc delimiters — there was a live violation at publish-testpypi.yml:67 interpolating ${DEV_VERSION} into Python source, fixed here since I was already in the file), actionlint + zizmor over .github/workflows/** (subsumes the existing CE026 candidate), and the sdist manifest guard paired with an explicit [tool.hatch.build.targets.sdist] allowlist.
Releases were tag-only: semantic-release runs with --no-vcs-release, so the repo had zero GitHub Releases. A published Release is what a Marketplace listing is cut from, and it is also where users look for notes. Create the Release explicitly after the tag pushes rather than dropping --no-vcs-release: semantic-release runs --no-push so the commit can be amended (uv.lock + the action.yml pin) and the tag re-pointed, which means the tag is not on the remote at the point semantic-release would publish. Notes come from the CHANGELOG section semantic-release just generated for the version, falling back to GitHub's generated notes with a ::warning:: rather than failing a release that has already pushed main and tags. The step authenticates with the release app token, not GITHUB_TOKEN, so the workflow's contents: read permission stays as documented. Also refresh the action.yml `version:` pin, which was stale at 0.8.6 — the in-release bump step only landed after the last release was cut, so @main consumers were installing 0.8.6. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Two findings, each confirmed independently by all three reviewers (gemini-3.1-pro, gpt-5.6-sol, opus). Write the release notes under RUNNER_TEMP instead of the repo root. The step wrote release-notes.md into the working tree two steps before `uv build`, and hatchling has no [tool.hatch.build.targets.sdist] config, so its default file selection swept the file into the published sdist. Verified by building: `coder_eval-0.8.9/release-notes.md` was present in the tarball. Adding the path to .gitignore would not have been a reliable fix -- the sdist already ships evalboard/node_modules/** despite that path being git-ignored. Make the step best-effort. main, the version tag, and the moving major tag are all pushed by the time it runs, so a transient `gh release create` failure aborted the job, skipped `Build wheel + sdist`, and left publish-pypi unrun -- stranding `@vN` on an action.yml pin whose version never reached PyPI. This mirrors the existing precedent in the same file: 522dbc7 ("tag only after PyPI publishes") made the GHCR steps continue-on-error for exactly this reason. Also narrows the header comment, which claimed a broader fallback than the `if m else ""` branch actually provided, and defers an sdist-contents guardrail to .claude/harness-candidates.md. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Reviewer items acted on: - Move "Publish GitHub Release" after "Build wheel + sdist" and "Upload dist for PyPI publish". Those are the last steps that can still fail for an already-tagged version, so a Release no longer announces a version whose artifacts never built, and a hung `gh` call can no longer eat the 15-minute job budget before the artifacts are safe. - Make the swallowed `continue-on-error` failure loud: a new "Flag missing GitHub Release" step re-raises it as an ::error annotation plus a run-summary block with the by-hand recovery command, without failing the job (which would skip the `needs: release` PyPI publish). - Extract the CHANGELOG notes slicing out of the `run:` heredoc into .github/scripts/release_notes.py, covered by tests/test_release_notes.py. Heredoc code is invisible to ruff, pyright, pytest and coverage, and this regex has three failure modes on a path that runs once per release against production main with no rehearsal (the prerelease dispatch skips the step). Tests pin the load-bearing \b (0.8.1 must not slice the v0.8.10 section), the \Z branch, re.escape, the empty-file fallback contract, and couple the regex to semantic-release's real rendering of CHANGELOG.md. - Pin encoding="utf-8" on every read_text/write_text in workflow-embedded Python (release.yml, publish-testpypi.yml). CHANGELOG.md carries non-ASCII, so a non-UTF-8 locale raised UnicodeDecodeError inside a continue-on-error step: no Release, still green. - Quote the publish-testpypi.yml heredoc delimiter (<<'PY') and read DEV_VERSION from os.environ instead of expanding it into the Python source. - Add tests/test_action_version_pin.py: action.yml's `version:` default must equal pyproject.toml's version, and the `# <-- kept in sync` sed anchor must be present and unique. Nothing detected the 0.8.6-vs-0.8.9 drift this PR hand-fixed, which would have had @v0 consumers installing a version other than the tag they pinned. - Fix the misleading step comment ("two steps below", "publishes to PyPI") and update the workflow header, which still omitted the GitHub Release from both the outputs list and the PRERELEASE exclusion list. - Record the accepted risk on the Release body: notes render squashed PR titles, so a first-party surface carries text reviewed as code, not markdown. Also note that Marketplace listing needs a one-time manual checkbox. - Widen the Makefile lint scope to .github/scripts/ so extracted release tooling is actually linted. Rescoped the deferred sdist note in .claude/harness-candidates.md: it claimed the sdist "already ships evalboard/node_modules/**" to PyPI. Verified false for published artifacts — the 0.8.9 and 0.8.2 sdists on PyPI are ~7.5 MB with zero node_modules entries, because CI never runs pnpm install. It reproduces only in a developed local worktree (135 MB, 8520 files), since hatchling honors just the root .gitignore. The live hazard is untracked files a workflow leaves at the root, which is what the $RUNNER_TEMP note guards. Also recorded CE032/CE033 and actionlint/zizmor as deferred candidates. Declined: swapping the app token for GITHUB_TOKEN + job-level contents: write. actions/checkout already persists the same app token in .git/config for every step in the job, so scoping it out of this one step's env buys no isolation while adding a second write credential. Rationale recorded in the step comment. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
f2329dd to
1e3feba
Compare

Why
Releases were tag-only.
release.ymlruns semantic-release with--no-vcs-release, so the repo has zero GitHub Releases despite tags up tov0.8.9. A published Release is what a GitHub Marketplace listing is cut from, and it is also where users look for notes.What
Publish a GitHub Release (
release.yml:229), after the tag pushes and before the wheel build:--no-vcs-releaserather than dropping it. semantic-release also runs--no-pushso the commit can be amended (uv.lock + theaction.ymlpin) and the tag re-pointed — the tag is therefore not on the remote at the point semantic-release would publish. Creating the Release explicitly after the push is the only correct ordering.$RUNNER_TEMP. Validated against every existing version in the realCHANGELOG.md, including the0.8.1vs0.8.10\bboundary and the oldest-version\Zterminator.--generate-noteswith a::warning::if that section can't be located.continue-on-error: true— see below.GITHUB_TOKEN, preserving the invariant documented atrelease.yml:42(workflow permissions staycontents: read; the app already carriescontents: write).Refresh the
action.ymlversion:pin — stale at0.8.6because the in-release bump step (release.yml:200) only landed after the last release was cut, so@mainconsumers were installing 0.8.6.Review fixes (3454cbf)
A multi-model review (gemini-3.1-pro, gpt-5.6-sol, Opus) found two issues in the first commit, each confirmed independently by all three reviewers. Both are fixed:
The notes file leaked into the published PyPI sdist. It was written to the repo root two steps before
uv build, andpyproject.tomldeclares no[tool.hatch.build.targets.sdist]section, so hatchling's default file selection swept it in. Confirmed by building:coder_eval-0.8.9/release-notes.mdwas present in the tarball. Now written under$RUNNER_TEMP, outside the build tree.Adding the path to
.gitignorewould not have been a reliable fix — the sdist already shipsevalboard/node_modules/**despite that path being git-ignored viaevalboard/.gitignore.The step was a hard-fail gate between the irreversible pushes and the PyPI publish.
main,v<version>, and the movingv<major>tag are all pushed before it runs, so a transientgh release createfailure aborted the job, skippedBuild wheel + sdist, and leftpublish-pypiunrun — stranding@vNon anaction.ymlpin whose version never reached PyPI. Nowcontinue-on-error: true, mirroring the precedent already in this file: 522dbc7 ("tag only after PyPI publishes") made the GHCR steps best-effort for exactly this reason.Two reviewers preferred a stronger version of fix 2 — moving the Release into its own job with
needs: [release, publish-pypi], so a Release can never announce a version that is not installable. Not done here: it is a larger restructuring, and it does not address the pre-existing window where the movingvNtag is force-moved before PyPI publishes either. Worth doing as its own change.Follow-ups (not in this PR)
uses: UiPath/coder_eval@v0appears inREADME.md:105,README.md:142,docs/CI_GATE.md:22and:67, but thev0tag does not exist yet —action.ymllanded after thev0.8.9tag. The next release creates it (release.yml:225-227); until then those docs point at a ref that fails.evalboard/node_modules/**to PyPI. Pre-existing and unrelated to this change, but it is real bloat plus third-party JS in a Python sdist. An sdist-contents guardrail is deferred in.claude/harness-candidates.md.action-dogfoodjob runsversion: local, so the default PyPI-install path a real consumer takes is untested.Test plan
make verifypassed before and after the review fixes.Publish GitHub Releaseis step 13 of 20, afterMove major action tag).$RUNNER_TEMPvariant re-verified end to end (correctghargv, repo root left clean).v0, and bump the pin to that version.🤖 Generated with Claude Code